Skip to content

[workspace] Upgrade vtk_internal to latest commit #23038

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

tyler-yankee
Copy link
Contributor

@tyler-yankee tyler-yankee commented May 23, 2025

Towards #22928.


This change is Reviewable

@tyler-yankee
Copy link
Contributor Author

-(status: do not review) +@BetsyMcPhail for feature review, please.

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(FYI @Aiden2244 you can probably close #22981 now.)

Reviewable status: LGTM missing from assignee BetsyMcPhail, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @tyler-yankee)

@tyler-yankee
Copy link
Contributor Author

ping @BetsyMcPhail it's probably a good idea to get this in before we run into conflicts with June's external upgrades :)

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers

@jwnimmer-tri jwnimmer-tri self-assigned this May 30, 2025
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: platform.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion


tools/workspace/vtk_internal/patches/upstream/io_import_errors.patch line 1 at r1 (raw file):
nit The verb "restore" here doesn't make sense in context. There is no information provided about what is being restored, or from where. The original comment in #21090 was also more descriptive -- this is a nested loader.

So maybe something like this for the patch description (with better line wrapping):

[vtk] Partially revert upstream commit de160283

Upstream deleted important error handling that TRI added, and
it should be put back. Until that happens, we can unrevert the
deletion using this patch file.

When this patch is upstreamed again next time, the VTK team should
be sure to also add a unit test so that it doesn't get reverted again.

Original description of the fix that got reverted: be sure to forward
warnings and errors from the loader that's nested inside the
importer back to the importer proper.

@tyler-yankee tyler-yankee force-pushed the vtk-internal-upgrade branch from 52ef1d3 to 35e3bc4 Compare May 30, 2025 18:29
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),BetsyMcPhail

@jwnimmer-tri jwnimmer-tri merged commit 6635e15 into RobotLocomotion:master May 30, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: fix This pull request contains fixes (no new features)
Development

Successfully merging this pull request may close these issues.

3 participants